Fixes issue #9347 - jump to live on seek to 0#753
Fixes issue #9347 - jump to live on seek to 0#753copybara-service[bot] merged 2 commits intoandroidx:mainfrom
Conversation
Overview@icbaker here's the pull request migrated from ExoPlayer codebase. To see this bug, checkout my branch @marcbaechinger this is the same seekTo(0) bug we still have, it is just a harder use case to work around. I'm looking at a longer fix that propagates a proper "use default" when using We have several use cases that trigger this bug, so we are using this fix in our branch now. Here's the logging from that branch along with my analysis and description of the fix and alternatives Analysis and loggingInitial First HLS playlist update sets the override based on the window computed start position Note at this point prepare is completed, the position is already reported correctly in the EventLogger event, so @Override
public long selectTracks(
@NullableType ExoTrackSelection[] selections,
boolean[] mayRetainStreamFlags,
@NullableType SampleStream[] streams,
boolean[] streamResetFlags,
long positionUs) {
Log.d(TAG, "selectTracks() - positionUs: " + positionUs
+ " preparePositionUs: " + preparePositionUs
+ " preparePositionOverrideUs: " + preparePositionOverrideUs);
if (preparePositionOverrideUs != C.TIME_UNSET && positionUs == preparePositionUs) {
Log.d(TAG, "selectTracks() - reset positionOverrideUs");
positionUs = preparePositionOverrideUs;
preparePositionOverrideUs = C.TIME_UNSET;
}
long resolvedPositionUs = castNonNull(mediaPeriod)
.selectTracks(selections, mayRetainStreamFlags, streams, streamResetFlags, positionUs);
Log.d(TAG, "selectTracks() - return positionUs: " + positionUs);Now the code to trigger the bug runs... Seek to 0 is logged, all good. And here the bug occurs. I think there are a couple of fixes to this, easiest first:
The longer fix (#2) is needed for the issue @marc discussed here: google/ExoPlayer#7975 (comment) For now, I've taken the easiest, which fixes issue google/ExoPlayer#9347 |
|
Can you please update this to be proposed against the |
Sure, I'll fix the messed up merge GitHub did then rebase and force push it |
stevemayhew
force-pushed
the
p-fix-issue-9347
branch
from
dcb6d3c to
55c433f
Compare
|
@icbaker I've rebased this branch and the branch with the test logging (https://github.com/stevemayhew/media/tree/x-logging-demo-issue-9347-seek-0-jumps-live) to The only use-case I've found the override code, in the Let me know if you'd like a live HLS test URL for this. |
Thanks for all the details and explanations! If I understand everything correctly, your change is mostly working, except that it causes a regression for this particular case? I was trying to craft a unit test in |
The change in this pull request is working perfectly. The problem I'm referencing to (RE: ERROR_CODE_IO_NETWORK_CONNECTION_FAILED recovery)) happened because of a previous pull request I made for this issue that remove the override code in the
Not surprised, when I completely removed the To reproduce the connection failed error (recovery logic is in the branch,
Without the code in |
|
Sorry to mix up multiple issues in this discussion.. But I think we can solve the LAN disconnect exhausted buffer in the |
Huh... you're right, but that feels unexpected! This either means we are missing a crucial test case or the code is indeed unneeded by now.
Was this meant to be
So just to summarize my understanding:
=> conclusion is that this PR is safe and at least fixed ExoPlayer#9347 Does that correspond to what you are saying? |
Correct, if you create this error and look at the logging... Here's the last retry that becomes a fatal error, note the position (end of the timeline basically): After the LAN cable is reconnected, then you click "retry" and the recovery code runs (basically just does a Seek to default: The Then the first playlist load establishes the window default start, at this point we have everything needed to establish the correct starting position, except the initial Things would go badly, if not for the code in So, if we remove that code (which test cases indicates we should), then we need to fix the initial prepare position so the
All correct, I think fixing the code to be able to remove the |
|
Thanks, so sounds like we can merge this PR. Do you want to add a test case for the ExoPlayer#9347 case? I think I saw one somewhere already but couldn't find it right now. |
|
I’m on vacation this week, pretty sure I have a branch with a start on one
in our (TiVo) fork of ExoPlayer…. I remember struggling a bit with mocking
the source update. As we iterate on a fix for this I’ll help increase the
test coverage, it’s a complicated part of the code
…On Tue, Nov 7, 2023 at 7:40 AM tonihei ***@***.***> wrote:
Thanks, so sounds like we can merge this PR. Do you want to add a test
case for the ExoPlayer#9347 case? I think I saw one somewhere already but
couldn't find it right now.
—
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADBF6H5MQ77ZEEHTT377GDYDJI5BAVCNFSM6AAAAAA6JJDSKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJYHEZDSNBSHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Just checking if you found the chance to add (or rather find) the test case? |
|
Yes my old attempt at unit test didn't really work... The logic includes code from
With these two the second |
|
I was able to create a test case with the problematic sequence in #9347 (initial start with non-zero default + later seekTo(0) + track selection). Will merge this change and add the test myself, thanks! |
tonihei
force-pushed
the
p-fix-issue-9347
branch
from
55c433f to
4af0938
Compare
|
Thanks @tonihei, getting the hang of the Let me known anything you need from me to close out the bug and complete the pull request, from our side this code fixes the issue. |
tonihei
force-pushed
the
p-fix-issue-9347
branch
from
4af0938 to
f505190
Compare
Using 0 as the unset prepare position is the root cause of a number of issues, as outliine in the ExoPlayer issue google/ExoPlayer#7975 The premise of this fix is that once the prepare override is used (the initial call to `selectTracks()`) it is never needed again, so simply invalidate it after use.
tonihei
force-pushed
the
p-fix-issue-9347
branch
from
f505190 to
7a70287
Compare
PiperOrigin-RevId: 590862514 (cherry picked from commit ab296ef)
|
I think my app may be affected by an issue in this area. I use a Forwarding player, and override the Play method and add a super.seekTo(0) as I had an issue where network drop outs would cause odd behaviour with the buffering, where it would continue to play a live steam when the network returned, and then, presumably, when the buffer was empty, jump to the latest part of the stream. For a network streaming radio, this didn't make any sense. It made more sense to jump to the live stream when the network returned. super.seekTo(0) for the most part, mostly fixes this. That said, there is still an edge case, where if the network is lost for an extended period, the radio stream will never resume, sit there forever doing something, not sure what. Playing/Pausing won't fix it. The only fix is to change "track", and return back, I guess my question is: Is there a fix here (or somewhere else) that might fix this (I'm currently seeing this in media3 1.2.0 ), Otherwise, is there something better I should be doing in my ForwardingPlayer to overcome this? Thanks. |
|
It’s not easy to hit, but yes error recovery that uses `prepare()` to retry
causes a track selection which combined with a seek to 0 can trigger it.
The fix is simple and in the pull request
We have error recovery when Exo reports a network error (socket error
usually) that retries and resumes at the failure position (if possible,
else to default live if not). Without this fix it doesn’t work 100%
…On Sun, Jan 14, 2024 at 9:20 AM Four Of Spades ***@***.***> wrote:
I think my app may be affected by an issue in this area. I use a
Forwarding player, and override the Play method and add a super.seekTo(0)
are I had an issue where network drop outs would cause odd behaviour with
the buffering, where it would continue to play a live steam when the
network returned, and then, presumably, when the buffer was empty, jump to
the latest part of the stream. For a network streaming radio, this didn't
make any sense. It made more sense to jump to the live stream when the
network returned. super.seekTo(0) mostly fixes this.
That said, there is still an edge case, where if the network is lost for
an extended period, the radio stream will never resume, sit there forever
doing something, not sure what. Playing/Pausing won't fix it. The only fix
is to change "track", and return back,
I guess my question is:
Is there a fix here (or somewhere else) that might fix this (I'm currently
seeing this in media3 1.2.0 ), Otherwise, is there something better I
should be doing in my ForwardingPlayer to overcome this?
Thanks.
—
Reply to this email directly, view it on GitHub
<#753 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADBF6ENMN3MKPBJCNMVNZ3YOQHXJAVCNFSM6AAAAAA6JJDSKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJRGAYTANRZGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
5 participants
Using 0 as the unset prepare position is the root cause of a number of issues, as outliine in the ExoPlayer issue google/ExoPlayer#7975
The premise of this fix is that once the prepare override is used (the initial call to
selectTracks()) it is never needed again, so simply invalidate it after use.